Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement mem::{zeroed,uninitialized} in terms of MaybeUninit. #62150

Merged
merged 1 commit into from
Jul 5, 2019
Merged

Implement mem::{zeroed,uninitialized} in terms of MaybeUninit. #62150

merged 1 commit into from
Jul 5, 2019
+12 −29

Conversation

alex
Copy link
Member

@alex alex commented Jun 26, 2019 •
edited
Loading

Refs #62061

r? @oli-obk

@rust-highfive
Copy link
Collaborator

r? @Kimundi

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 26, 2019
@alex
Copy link
Member Author

alex commented Jun 26, 2019

r? @oli-obk

@rust-highfive rust-highfive assigned oli-obk and unassigned Kimundi Jun 26, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Jun 26, 2019

looks fine to me from a compiler perspective

maybe someone from @rust-lang/libs wants to look at the MaybeUninit usage?

cc @RalfJung

@RalfJung
Copy link
Member

Fine from a MybeUninit perspective.

Let's ask perf though, just to be sure.

@bors try

@bors
Copy link
Contributor

bors commented Jun 26, 2019

⌛ Trying commit 5c150fc with merge c6c5b77...

bors added a commit that referenced this pull request Jun 26, 2019
@bors
Auto merge of #62150 - alex:mem-uninit-refactor, r=<try>
c6c5b77 
Implement mem::{zeroed,uninitialized} in terms of MaybeUninit.

Refs #62061

r? @oli-obk
@RalfJung
Copy link
Member

RalfJung commented Jun 26, 2019 •
edited
Loading

Note that the init intrinsic is unused now, but we won't remove it yet as it will be used again in a follow-up PR.

@RalfJung
Copy link
Member

Maybe it makes sense to mark the init intrinsic as deprecated already in this PR? Seems to be in-line with the other changes here.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 26, 2019

good idea

@SimonSapin
Copy link
Contributor

Why remove the calls to panic_if_uninhabited?

@Centril
Copy link
Contributor

Centril commented Jun 26, 2019

@SimonSapin They happen in .assume_init().

@alex
Copy link
Member Author

alex commented Jun 26, 2019

Hmm, simply sticking a #[rustc_deprecated()] on the fn init results in an error that rustc_deprecated attribute must be paired with either stable or unstable attribute. The whole module is marked unstable though. Is there an appropriate idiom here? Simply mark the function unstable again?

@Centril
Copy link
Contributor

Centril commented Jun 26, 2019

r? @RalfJung

Feel free to review this since the PR doesn't change anything observably.

@rust-highfive rust-highfive assigned RalfJung and unassigned oli-obk Jun 26, 2019
@SimonSapin
Copy link
Contributor

@alex Yes, items without a #[stable] or #[unstable] attribute “inherit” that of their parent item. (At least when the parent is a module.) So duplicating that attribute to satisfy the lint does not change the stability of the function.

@alex alex force-pushed the mem-uninit-refactor branch from 5c150fc to b3b2d48 Compare June 26, 2019 12:40
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log. 
travis_time:end:036aa8a0:start=1561552896655659899,finish=1561552898843198151,duration=2187538252
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:57:31] 
[00:57:31] ---- [ui] ui/init-unsafe.rs stdout ----
[00:57:31] diff of stderr:
[00:57:31] 
[00:57:31] + warning: use of deprecated item 'std::intrinsics::init': no longer used by rustc, will be removed - use MaybeUnint instead
[00:57:31] +    |
[00:57:31] + LL | use std::intrinsics::{init};
[00:57:31] +    |                       ^^^^
[00:57:31] +    |
[00:57:31] +    |
[00:57:31] +    = note: #[warn(deprecated)] on by default
[00:57:31] + 
[00:57:31] + warning: use of deprecated item 'std::intrinsics::init': no longer used by rustc, will be removed - use MaybeUnint instead
[00:57:31] +    |
[00:57:31] +    |
[00:57:31] + LL |     let stuff = init::<isize>();
[00:57:31] + 
[00:57:31] 1 error[E0133]: call to unsafe function is unsafe and requires unsafe function or block
[00:57:31] 2   --> $DIR/init-unsafe.rs:7:17
[00:57:31] 3    |
[00:57:31] 3    |
[00:57:31] 
[00:57:31] 
[00:57:31] The actual stderr differed from the expected stderr.
[00:57:31] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/init-unsafe/init-unsafe.stderr
[00:57:31] To update references, rerun the tests and pass the `--bless` flag
[00:57:31] To only update this specific test, also pass `--test-args init-unsafe.rs`
[00:57:31] error: 1 errors occurred comparing output.
[00:57:31] status: exit code: 1
[00:57:31] status: exit code: 1
[00:57:31] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/init-unsafe.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/init-unsafe" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/init-unsafe/auxiliary" "-A" "unused"
[00:57:31] ------------------------------------------
[00:57:31] 
[00:57:31] ------------------------------------------
[00:57:31] stderr:
[00:57:31] stderr:
[00:57:31] ------------------------------------------
[00:57:31] warning: use of deprecated item 'std::intrinsics::init': no longer used by rustc, will be removed - use MaybeUnint instead
[00:57:31]    |
[00:57:31] LL | use std::intrinsics::{init};
[00:57:31]    |                       ^^^^
[00:57:31]    |
[00:57:31]    |
[00:57:31]    = note: #[warn(deprecated)] on by default
[00:57:31] 
[00:57:31] warning: use of deprecated item 'std::intrinsics::init': no longer used by rustc, will be removed - use MaybeUnint instead
[00:57:31]    |
[00:57:31]    |
[00:57:31] LL |     let stuff = init::<isize>(); //~ ERROR call to unsafe function is unsafe
[00:57:31] 
[00:57:31] error[E0133]: call to unsafe function is unsafe and requires unsafe function or block
[00:57:31]   --> /checkout/src/test/ui/init-unsafe.rs:7:17
[00:57:31]    |
[00:57:31]    |
[00:57:31] LL |     let stuff = init::<isize>(); //~ ERROR call to unsafe function is unsafe
[00:57:31]    |                 ^^^^^^^^^^^^^^^ call to unsafe function
[00:57:31]    |
[00:57:31]    = note: consult the function's documentation for information on how to avoid undefined behavior
[00:57:31] error: aborting due to previous error
[00:57:31] 
[00:57:31] For more information about this error, try `rustc --explain E0133`.
[00:57:31] 
---
[00:57:31] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:521:22
[00:57:31] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[00:57:31] 
[00:57:31] 
[00:57:31] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:57:31] 
[00:57:31] 
[00:57:31] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:57:31] Build completed unsuccessfully in 0:52:45
---
travis_time:end:099e4630:start=1561556362959203865,finish=1561556362963903619,duration=4699754
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0fcb578c
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:241f6aea
travis_time:start:241f6aea
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:02c4dc68
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@alex
Copy link
Member Author

alex commented Jun 26, 2019

Oof. Is the correct fix just to #![allow(deprecated) for that test?

@Centril
Copy link
Contributor

Centril commented Jun 26, 2019

I think we should do the steps @oli-obk outlined in #62061 (comment). If we do that, then imo we can just remove intrinsics::init here and now.

@RalfJung
Copy link
Member

That seems like a change with enough potential for problems that I wouldn't want to block removing uninit on it.

Is the correct fix just to #![allow(deprecated) for that test?

Yes.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 26, 2019

That seems like a change with enough potential for problems that I wouldn't want to block removing uninit on it.

I think the idea was not to block this PR, but to just nix init here, too and think about future steps in separate PRs.

@RalfJung
Copy link
Member

Well that would be fine for me. It would commit ourselves to not follow this plan and instead do the "proper" thing right away. I just wanted to not stand in the way of making MaybeUninit::zeroed a const fn ASAP since some people have now waited for it for more than half a year.

But I will never argue against nixing an intrinsic such as init. ;)

@alex alex force-pushed the mem-uninit-refactor branch from b3b2d48 to b366f2b Compare June 26, 2019 22:49
@alex
Copy link
Member Author

alex commented Jun 27, 2019

Ok, should be green now.

I don't know nearly enough about the &mut in const fn issue to take that on unfortunately.

@RalfJung
Copy link
Member

LGTM, unless @Centril and @oli-obk want to push through with the idea of nixing init as well?

We could easily do that in a separate PR though.

@bors bors merged commit e4f250e into rust-lang:master Jul 5, 2019
@alex alex deleted the mem-uninit-refactor branch July 5, 2019 15:38
@EPashkin EPashkin mentioned this pull request Jul 12, 2019
Merged
This was referenced Jul 17, 2019
Closed
Closed
@RalfJung RalfJung mentioned this pull request Jul 19, 2019
Closed
@RalfJung
Copy link
Member

Seems like this PR caused SIGILL in a bunch of libraries (see the backlinks GH inserted above), but it also seems like these libraries all had UB -- so tiny changes in the implementations of these methods can lead to arbitrary changes in behavior.

@ishitatsuyuki
Copy link
Contributor

Based on what described in amethyst/amethyst#1808 (comment), I suggest that this change to be backed out for at least one stable release. This kind of UB existed in old dependencies that are no longer maintained, therefore some crates have to migrate their dependency to the latest version to mitigate the issue, and we need some time for the ecosystem to do that major version upgrades.

@RalfJung
Copy link
Member

RalfJung commented Jul 20, 2019 •
edited
Loading

@ishitatsuyuki a closed PR is not a good place for a new discussion. Could you open a new issue and Cc me? Then I will nominate it for lang team discussion.

@ishitatsuyuki ishitatsuyuki mentioned this pull request Jul 20, 2019
Closed
@mateon1 mateon1 mentioned this pull request Jul 22, 2019
Closed
ishitatsuyuki added a commit to ishitatsuyuki/rust that referenced this pull request Aug 10, 2019
@ishitatsuyuki
Revert "Rollup merge of rust-lang#62150 - alex:mem-uninit-refactor, r…
2358e3e 
…=RalfJung"

This reverts commit 1d45156, reversing
changes made to 0f92eb8.
bors added a commit that referenced this pull request Aug 11, 2019
@bors
Auto merge of #63343 - ishitatsuyuki:revert-62150, r=RalfJung
2b78e10 
Back out #62150

Ref: #62825

cc @RalfJung
@bjorn3 bjorn3 mentioned this pull request Oct 9, 2019
Merged
@RalfJung RalfJung mentioned this pull request Feb 17, 2020
Merged
@RalfJung RalfJung mentioned this pull request Mar 11, 2020
Merged
Centril added a commit to Centril/rust that referenced this pull request Mar 17, 2020
@Centril
Rollup merge of rust-lang#69922 - RalfJung:less-intrinsic, r=oli-obk
8582b04 
implement zeroed and uninitialized with MaybeUninit

This is the second attempt of doing such a change (first PR: rust-lang#62150). The last change [got reverted](rust-lang#63343) because it [caused](rust-lang#62825) some [issues](rust-lang#52898 (comment)) in [code that incorrectly used these functions](AltF02/x11-rs#99).

Since then, the [problematic code has been fixed](AltF02/x11-rs#101), and rustc [gained a lint](rust-lang#63346) that is able to detect many misuses of these functions statically and a [dynamic check that panics](rust-lang#66059) instead of causing UB for some incorrect uses.

Fixes rust-lang#62825
Centril added a commit to Centril/rust that referenced this pull request Mar 17, 2020
@Centril
Rollup merge of rust-lang#69922 - RalfJung:less-intrinsic, r=oli-obk
ffec535 
implement zeroed and uninitialized with MaybeUninit

This is the second attempt of doing such a change (first PR: rust-lang#62150). The last change [got reverted](rust-lang#63343) because it [caused](rust-lang#62825) some [issues](rust-lang#52898 (comment)) in [code that incorrectly used these functions](AltF02/x11-rs#99).

Since then, the [problematic code has been fixed](AltF02/x11-rs#101), and rustc [gained a lint](rust-lang#63346) that is able to detect many misuses of these functions statically and a [dynamic check that panics](rust-lang#66059) instead of causing UB for some incorrect uses.

Fixes rust-lang#62825
Centril added a commit to Centril/rust that referenced this pull request Mar 17, 2020
@Centril
Rollup merge of rust-lang#69922 - RalfJung:less-intrinsic, r=oli-obk
7a7ca82 
implement zeroed and uninitialized with MaybeUninit

This is the second attempt of doing such a change (first PR: rust-lang#62150). The last change [got reverted](rust-lang#63343) because it [caused](rust-lang#62825) some [issues](rust-lang#52898 (comment)) in [code that incorrectly used these functions](AltF02/x11-rs#99).

Since then, the [problematic code has been fixed](AltF02/x11-rs#101), and rustc [gained a lint](rust-lang#63346) that is able to detect many misuses of these functions statically and a [dynamic check that panics](rust-lang#66059) instead of causing UB for some incorrect uses.

Fixes rust-lang#62825
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@RalfJung RalfJung

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.